feat: add memory primitive by opieter-aws · Pull Request #1096 · strands-agents/sdk-typescript

opieter-aws · 2026-05-22T19:51:06Z

Description

Related Issues

Documentation PR

Type of Change

Bug fix
New feature
Breaking change
Documentation update
Other (please describe):

Testing

How have you tested the change?

I ran npm run check

Checklist

I have read the CONTRIBUTING document
I have added any necessary tests that prove my fix is effective or my feature works
I have updated the documentation accordingly
I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
My changes generate no new warnings
Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2026-05-22T19:59:02Z

Issue: The PR description is empty — no description of the feature, no linked issue, no use cases, no API design rationale, and all checklist items are unchecked.

Suggestion: This PR introduces a significant new public primitive (MemoryManager, KnowledgeStore, etc.). Per the project's contribution guidelines and API bar raising practices, the description should include:

A clear explanation of what this feature does and why it's needed
Expected use cases with example code snippets demonstrating usage
Complete API signatures with default parameter values
A linked issue for the feature request
Design rationale — why a dedicated memoryManager config field vs. a generic plugin?

This PR should also carry a needs-api-review label given that it introduces new public classes and interfaces that customers will use directly.

github-actions · 2026-05-22T19:59:13Z

Assessment: Request Changes

This PR introduces a MemoryManager primitive for cross-session knowledge retrieval/storage. The implementation is well-structured and follows existing SDK patterns (Plugin interface, co-located tests, barrel exports), but there are important API design questions that should be resolved before merging.

Review Themes

API Justification: The dedicated memoryManager config field doesn't differentiate itself from a generic plugin (no-op initAgent). Either add meaningful agent-level behavior or keep this as a plugin until the API is more mature.
Type Safety: Index signatures on SearchOptions and type casts in tool callbacks weaken TypeScript's guarantees — consider tightening.
Error Handling: Silent swallowing of write failures in store() risks data loss without caller awareness.
Incomplete Surface: delete is defined on KnowledgeStore but unreachable through the manager; dead API surface creates confusion.
Testing Gaps: Tool callbacks (the integration point where bugs are most likely) are never invoked in tests.
PR Process: Missing description, linked issue, use cases, and needs-api-review label for a significant new public API.

The core design is solid and the test coverage for direct method calls is thorough. Addressing the API design questions and adding tool integration tests would make this ready to merge.

github-actions · 2026-05-26T14:23:39Z

+   *
+   * @param _agent - The agent to register hooks with
+   */
+  initAgent(_agent: LocalAgent): void {}


Issue: Since initAgent is a no-op, MemoryManager only uses the getTools() extension point — which means it's functionally identical to passing it via plugins: [memoryManager]. The dedicated memoryManager config field on AgentConfig adds API surface without differentiated behavior.

Suggestion: Either:

Add meaningful initAgent behavior (e.g., registering a BeforeModelCallEvent hook to auto-inject relevant context from memory into the system prompt or messages), which would justify the dedicated field, OR

Remove the dedicated field for now and document MemoryManager as a regular plugin (plugins: [new MemoryManager({...})]) until the feature is more mature. This keeps the Agent config surface minimal.

The session manager justifies its dedicated field because it registers multiple hooks in initAgent (AfterInvocation, MessageAdded, etc.). A parallel justification is needed here.

initAgent will be used for injection (BeforeInvocationEvent), ingestion (AfterInvocationEvent), and eviction-triggered ingestion. These are planned for follow-up PRs.

github-actions · 2026-05-26T14:23:45Z

Assessment: Request Changes

The MemoryManager primitive is well-implemented — clean types, proper Plugin interface usage, good error handling in store(), and solid test coverage of the core methods. However, there are a few substantive issues to address before merging.

Review Categories

API Design: The dedicated memoryManager config field lacks justification since initAgent is a no-op — this makes it indistinguishable from a generic plugin. Either add meaningful lifecycle behavior or remove the dedicated field until the feature matures.
Error Resilience: store_memory tool uses Promise.all (fail-fast) wrapping store() calls that can throw — this creates an inconsistency with the Promise.allSettled resilience pattern used within store() itself.
Documentation Accuracy: TSDoc claims "automatic context injection" which doesn't exist in the implementation.
Testing Gap: Tool callbacks are never invoked in tests, leaving the marshalling/formatting logic untested.

The underlying types (KnowledgeStore, KnowledgeEntry, StoreConfig) are well-designed and the implementation is clean. Addressing the API justification and the Promise.all bug would make this ready.

github-actions · 2026-05-26T14:48:44Z

Assessment: Comment (prior "Request Changes" items are largely addressed)

The author has addressed the majority of prior review feedback — Promise.allSettled in store_memory, TSDoc accuracy, SearchOptions cleanup, MemorySearchOptions addition, and type-safe callbacks. The code is clean and well-structured.

Remaining Items

API Consistency: memoryManager has a dedicated config field but isn't exposed as a public property on Agent (unlike sessionManager), preventing programmatic access.
Edge Case: Truthiness check on input.limit silently drops a value of 0.
Testing: Tool callbacks are untested — their formatting/mapping logic is an integration boundary worth covering.
Error Behavior: Silent empty results on invalid store names in search() vs. throwing in store() — a warning log would help users catch typos.

The implementation is solid and the author's reply about future initAgent hooks is a reasonable approach. These remaining items are mostly incremental improvements.

github-actions · 2026-05-26T19:36:46Z

Assessment: Comment

The MemoryManager implementation is well-structured and the author has addressed all prior substantive feedback — type safety, error handling, TSDoc accuracy, and API surface are all in good shape. Two minor items remain.

Remaining Items

Documentation: AGENTS.md is updated (stale entries removed) but doesn't add the new memory/ directory, which is required per the repo's own guidelines.
Testing: Tool callbacks contain formatting/mapping logic that isn't covered by any test. The core methods are well-tested but the tool integration boundary is still a gap.

Clean implementation that follows existing SDK patterns (Plugin interface, structured logging, barrel exports, co-located tests).

github-actions · 2026-05-28T20:00:47Z

Issue: This PR is 160 files changed (~5,655 insertions, ~14,699 deletions) bundling the new memory feature with multiple unrelated breaking changes:

Removal of InterventionHandler/InterventionRegistry (entire system deleted)
Removal of ToolCaller/ToolCallerProxy/agent.tool accessor
Removal of agent.isInvoking public getter
Removal of per-invocation limits (_validateLimits, _checkLimits)
Removal of Sandbox/PosixShellSandbox exports
Removal of ToolNotFoundError, BedrockCacheConfig, MCP client types
strands-py-wasm → strands-py rename
WIT interface changes
acquireLock refactor to using Disposable pattern

Each of these is a breaking change that removes public API surface. Bundling them together makes it impossible to review the memory feature in isolation, creates a massive risk of merge conflicts, and makes it very difficult to revert any single change if issues arise.

Suggestion: Split this into separate PRs:

This PR — scoped to only the memory/ feature (the 6 new/modified files shown in the PR)
Separate PR(s) — for the breaking removals (interventions, tool-caller, limits, sandbox) with proper BREAKING CHANGE: commit footers and semver consideration

If the removals are already planned/tracked elsewhere, rebase this branch to remove them from the diff.

github-actions · 2026-05-28T20:00:49Z

+        throw new Error('MemoryManager: storeToolConfig targets no writable stores')
+      }
+
+      if (config.storeToolConfig === true && resolved.length > 1 && !toolConfig.stores) {


Issue: storeToolConfig: true enforces a safety check that throws when multiple writable stores exist (preventing ambiguous multi-store writes), but storeToolConfig: {} bypasses this check entirely — even though both represent "enable the store tool with defaults."

// This throws: "must specify `stores` when multiple writable stores" new MemoryManager({ stores: [writableA, writableB], storeToolConfig: true }) // This silently writes to BOTH stores — no validation new MemoryManager({ stores: [writableA, writableB], storeToolConfig: {} })

The guard on line 82 checks config.storeToolConfig === true (strict equality), so the empty object escapes it.

Suggestion: Change the check to apply whenever toolConfig.stores is unspecified, regardless of whether the input was true or {}:

if (resolved.length > 1 && !toolConfig.stores) { throw new Error( 'MemoryManager: storeToolConfig must specify `stores` when multiple writable stores are configured' ) }

This closes the escape hatch and makes the two equivalent inputs behave identically.

github-actions · 2026-05-28T20:00:51Z

Assessment: Request Changes

The memory feature itself is well-implemented and most prior feedback has been addressed. However, the PR bundles 160 files of unrelated breaking changes (interventions removal, tool-caller removal, limits removal, sandbox removal, renames) alongside the new feature, which should be split.

Review Categories

Critical — PR scope: 160 files, ~20k lines changed. Multiple breaking public API removals are bundled with the new feature. Should be split into focused PRs.
Important — Validation gap: storeToolConfig: true and storeToolConfig: {} have different validation behavior for multi-writable-store scenarios despite identical semantic intent.
Important — Documentation: AGENTS.md directory structure not updated for new memory/ directory (persisting from prior round).
Important — Testing: Tool callbacks contain formatting/mapping logic untested at the integration boundary (persisting from prior round).

The memory implementation itself (types, MemoryManager, tests) is clean and follows SDK patterns well.

…s-agents#1096)

opieter-aws had a problem deploying to manual-approval May 22, 2026 19:51 — with GitHub Actions Error

opieter-aws temporarily deployed to manual-approval May 22, 2026 19:51 — with GitHub Actions Inactive

github-actions Bot added the strands-running <strands-managed> Whether or not an agent is currently running label May 22, 2026